Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CR 2375: TimeType.to_readable should display microsecond precision. #197

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

Ahmad-Bamba
Copy link
Contributor

Related Issue(s) 2375
Has Unit Tests (y/n) n
Documentation Included (y/n) n

Change Description

This change makes the TimeType.to_readable function use datetime.isoformat to convert the datetime object to a human readable format. In doing so, fprime-gds outputs channel.log entries with microsecond precision.

Rationale

The fundamental philosophy that must be applied to the ground system is that any conversion (time, DN->EU, units, etc.) should never decrease the resolution of the source data.

Testing/Review Recommendations

I tested the changes locally by first following the fprime hello world tutorial. Then I installed the local version of fprime-tools, and ran the example project with fprime-gds. This produced the following output that I've attached below showing no more accuracy loss in channel.log:

image

Compared with before this change:

image

Future Work

This seems like too a small change to make a unit test for, but I could think of a good way to design one if requested.

Copy link
Contributor

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thank you!

Could we just force the timespec dt.isoformat(timespec="microseconds") ? Default seems to be auto which falls back to microseconds in our case, but you never know, and it's also easier to read/understand that way.

You're also going to need to format the file for it to pass CI, and add isoformat to https://github.com/fprime-community/fprime-tools/blob/devel/.github/actions/spelling/expect.txt for spelling

@Ahmad-Bamba
Copy link
Contributor Author

Feel free to let me know if there is anything else that can be done to improve this

@thomas-bc
Copy link
Contributor

@Ahmad-Bamba could you please also push the timespec="microseconds" change?

@Ahmad-Bamba
Copy link
Contributor Author

Ah, I neglected to actually git add that file. My bad

…pe.py with Black. Add isoformat to spellcheck CI"
@Ahmad-Bamba
Copy link
Contributor Author

Should be good. Sorry about that!

@thomas-bc
Copy link
Contributor

@LeStarch would you put a second set of eyes on this ?

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great solution. Much better than what we had. I confirmed that from Python 3.6 through 3.11 the documentation backs-up the change (this call is supported in all versions)!

@LeStarch LeStarch merged commit b1c4815 into nasa:devel Mar 11, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants